Jump to content

Wikipedia:Bots/Requests for approval/MusikBot II 2

Add links
From Wikipedia, the free encyclopedia
The following discussion is an archived debate. Please do not modify it. To request review of this BRFA, please start a new section at WT:BRFA. The result of the discussion was  Approved.

MusikBot II 2

[edit]

Operator: MusikAnimal (talk · contribs · SUL · edit count · logs · page moves · block log · rights log · ANI search)

Time filed: 03:23, Monday, September 24, 2018 (UTC)

Automatic, Supervised, or Manual: Automatic

Programming language(s): Typically I use Ruby, but here it may have to be PHP or possibly Node.js.

Source code available: GitHub

Function overview: Syncs Wikipedia:Geonotice/list.json (to be created, fully-protected) with MediaWiki:Gadget-geonotice-list.js.

Links to relevant discussions (where appropriate): Special:PermaLink/862124571#Geonotices closed discussion, support of usage at Wikipedia talk:Interface administrators (see also RFC for IAdmins at top of that page allowing bot access where bot operator is also an IAdmin)

Edit period(s): Continuous

Estimated number of pages affected: 1

Namespace(s): MediaWiki

Exclusion compliant (Yes/No): No, not applicable.

Adminbot (Yes/No): Yes

Function details: First, some background: With the advent of the interface administrator user group, sysops can no longer edit MediaWiki:Gadget-geonotice-list.js. Many of these users are not particularly tech-savvy, and have no use for editing site-wide JS outside configuring geonotices for outreach purposes, etc. The configuration is literally just a JavaScript object, with key/value pairs. Using a JSON page then makes much more sense, which they'd be able to edit. However currently we cannot put JSON pages behind ResourceLoader (phab:T198758), so for performance reasons we need to continue to maintain the JS page. The proposed workaround is have a bot sync a JSON page with the JS page. This is in our best interests for security reasons (fewer accounts with access to site JS), but also JSON is easier to work with and gives you nice formatting, hence less prone to mistakes.

Implementation details:

  1. Check the time of the last edit to Wikipedia:Geonotice/list.json.
  2. If it is after the time of the last sync by the bot (tracked by local caching), process the JSON.
  3. Perform validations, which include full JSON validation, validating the date formats, country code (going off of ISO 3166), and format of the corners.
  4. If validations fail, report them at User:MusikBot II/GeonoticeSync/Report (example) and do nothing more.
  5. If validations pass, build the JS and write to MediaWiki:Gadget-geonotice-list.js (example), and update the report stating there are no errors (example).

The comment block at the top of MediaWiki:Gadget-geonotice-list.js can be freely edited. The bot will retain this in full.

Discussion

[edit]

Per Xaosflux I've preemptively started this BRFA. I haven't done any coding but I think this is a good time to discuss implementation details. Concerns that come to mind:

  • What to do when there are syntax errors. The native JSON editor should mean admins won't introduce syntax errors, because it won't even let you save. But, it can happen -- say the admin ironically has JavaScript disabled. As a safeguard, the bot can validate the JSON, too (easy, thanks to existing libraries). Similar to User:MusikBot/PermClerk/Report, the bot could have a status report page, transcluded at Wikipedia talk:Geonotice/list.json. This way they can get some debugging info should something go wrong. If we want to get real fancy, the bot could also report when the configuration doesn't match the expected format, as described in the comments at MediaWiki:Gadget-geonotice-list.js. I think that would a nice feature, but not a requirement.
  • After deployment, we'd need to update the JS page to clearly say it should not be edited directly. We could do a two-way syncing, but I'd prefer not to, just to keep it simple.
  • I can confirm MusikBot II's account is secure and 2FA is enabled (with some caveats). The bot solution still puts us on the winning end, as there will be fewer int-admin accounts than if we added it to all who manage geonotices.
  • Anything else? — MusikAnimal talk 03:23, 24 September 2018 (UTC)[reply]
  • @MusikAnimal: for the directional sync concerns, a defined "section" (delineated by comments) should be the only area edited - this section should be marked "do not edit directly" - and the bot should only edit within the section. This way if other changes to the page are needed they won't interfere. — xaosflux Talk 04:25, 24 September 2018 (UTC)[reply]
  • I'd like to see some tests over at testwiki that can be used to demonstrate the edits. — xaosflux Talk 04:25, 24 September 2018 (UTC)[reply]
  • Syntax errors could still occur in the data - will you validate this as well? For example putting start/end dates in their own cells, validate that this is date data and not something else? Everything should be validated (e.g. this should not be a route to inject new javascript). — xaosflux Talk 04:25, 24 September 2018 (UTC)[reply]
    • Perhaps make the mock-up json page to demonstrate? — xaosflux Talk 04:25, 24 September 2018 (UTC)[reply]
    • JS injection shouldn't be possible, unless there are vulnerabilities in Geonotice itself. I would hope it doesn't use eval on the strings. Arbitrary code (e.g. begin: alert('foo!') isn't valid JSON and hence would fail on the initial validation (and the MediaWiki JSON editor won't let you save it, either). We can still validate it ourselves, to be sure. As I said this would be a nice feature. I don't know that I want to validate things like the country, though. We could validate the 'begin'/'end' date format, in particular, but for everything else I think the bot will just look for permissible keys and the data type of the values ('country' is a string, 'corners' is an array of two arrays, each with two integers). — MusikAnimal talk 16:41, 24 September 2018 (UTC)[reply]
      • Injection would be if you accepted arbitrary "text" and just made it js, where the text could contain characters that would terminate the text field and then continue along in javascript. — xaosflux Talk 17:11, 24 September 2018 (UTC)[reply]
  • For the JSON page, not the bot: we'll also have to move the normal explanation text into an editnotice or regular notice, since comments are stripped on save for pages with the JSON content model. Enterprisey (talk!) 23:32, 24 September 2018 (UTC)[reply]
  • Got a prototype working, see Special:Diff/861109475. There are quotations around all the keys, but JavaScript shouldn't care. Maybe we should test against testwiki's Geonotice to be sure. This does mean the rules have changed -- you don't need to escape single quotes ', but you do for double quotation marks ". This is just a consequence -- that's how JSON wants it. I think single quotes are probably more commonly used in the geonotice text anyway, so this might be a welcomed change. The bot could find/replace all "'s to ', but this would be purely cosmetic and error-prone when it is not really needed. Other formatting has changed, mostly whitespace. Also in the edit summary we're linking to the combined diff of all edits made to the JSON page since the last sync. That way we can easily verify it was copied over correctly. We do loose attribution here (as opposed to linking to individual diffs), but I think that's okay? Source code (work in progress) is on GitHub. I've made this task translatable, should other wikis be interested in it. I'm going to stop here until the bot proposal discussion has closed. — MusikAnimal talk 04:55, 25 September 2018 (UTC)[reply]
    I agree with the quoting change. You may want to specify the number of edits if it's more than one, but I don't know if that's required for attribution. (And it's displayed on the diff page anyway.) Enterprisey (talk!) 06:16, 25 September 2018 (UTC)[reply]
  • I started adding some 'directions' at Template:Editnotices/Page/User:MusikBot II/GeonoticeSync/Test config.json, please fill out with more directions, requirements, etc. As far as attribution, in the edit request at least pipe to the name of the source page to make it clear where the source is without having to follow the diff. — xaosflux Talk 15:21, 28 September 2018 (UTC)[reply]
  • Update I've resumed work on this and am ready for more feedback. The current implementation is described in the "function details" above. I still need to work on filling out Template:Editnotices/Page/User:MusikBot II/GeonoticeSync/Test config.json, please feel free to help. That page will be moved to Template:Editnotices/Page/Wikipedia:Geonotice/list.json when we're ready to go live.

    For validations, see Special:PermaLink/863494086 for an example invalid config (with lots of errors!) and Special:PermaLink/863494234 for generated report. A few notes:

    • I'm using Ruby internal methods to tell if the date is valid. This works for "Invalid date" or "35 January 2018 00:00 UTC" but not for invalid month names as with "15 Foobar 2018 00:00 UTC". Going by some logic I don't understand it chooses some other valid month. I could use regular expressions to ensure the month names are valid, but I want this bot task to work in other languages where I assume they're able to put in localized month names, if not a different format entirely (which Ruby should still be able to validate). Anyway I think this is fine. There were no validations before, after all :)
    • Validating the country code actually works! It's going off of the ISO 3166 spec, which is what's advertised as the valid codes Geonotice accepts.
    • Coordinates are validated by ensuring there are two corners, and each with two values (lat and lng), and that the values are floats and not integers or strings.
    • The keys of each list item are also validated, ensuring they only include "begin", "end", "country", and either "corners" or "text".
    • I added code to check if they escaped single quotations (as with \'), since Geonotice admins probably are used to doing this. Amazingly, MediaWiki won't even let you save the JSON page if you try to do this, as indeed it is invalid JSON. So turns out no validation is needed for this, or any other JSON syntax errors for that matter. This should mean we don't need to worry about anyone injecting malicious code.
    • The comment block at the top of the JS page is retained and can be freely edited.
    • Back to the issue of attribution in the edit summary, I went with Xaosflux's recommendation and am using a combined diff link, piped with the title of the JSON page. I'm not sure it's worth the hassle of adding in comments in the generated JS code directly, but let me know if there are any strong feelings about that.

Let me know if there's anything else I should do, or if we're ready for a trial! — MusikAnimal talk 04:35, 11 October 2018 (UTC)[reply]

MediaWiki won't let you save the page with invalid JSON even if you turn off JS or use the API, right? Because if it does you may want to validate for that case. Enterprisey (talk!) 04:43, 11 October 2018 (UTC)[reply]
Luckily it's server side. It shows the error "Invalid content data", even if you have JS turned off. I haven't tested the API yet, but if it does work it's probably a bug in MediaWiki :) — MusikAnimal talk 16:38, 11 October 2018 (UTC)[reply]
But I should clarify, the bot does validate JSON content, but I haven't tested to see if this works because I am unable to create invalid JSON :) At any rate, we would not end up in a situation where an invalid JS object is written to MediaWiki:Gadget-geonotice-list.js, because the core JSON methods that we're using would error out before this happens. — MusikAnimal talk 20:15, 11 October 2018 (UTC)[reply]
This seems fine to me. My main concern (and it is fairly minor) is making sure the bot's role is clear in documentation/notices, and that people will know how to look for errors if something doesn't get updated because validation failed (because there won't be immediate user feedback as there is with the basic MW-side JSON validation). I'm giving this for a two-week trial, pending completion of the editnotice(s) and related pages and granting of the i-admin flag; based on history, that should allow for at least a handful of edits to test with, but feel free to extend if more time is required. Approved for trial (14 days). Please provide a link to the relevant contributions and/or diffs when the trial is complete. — Earwig talk 05:51, 14 October 2018 (UTC)[reply]
@The Earwig: will this be trialing on the actual pages or in userspace? Ping me if you need a flag assigned for trialing. — xaosflux Talk 00:06, 15 October 2018 (UTC)[reply]
@Xaosflux: My intention is for a full trial. I saw there were already reasonable tests done in the userspace, so given that MA feels comfortable working with the actual pages now, I'm fine with that too. As for the IA flag, it's not clear to me from the policy whether we can do that here or a request needs to be explicitly made to BN? I would prefer MA post something to BN to be safe, but I suppose one interpretation of the policy would let you grant it immediately without the waiting period. — Earwig talk 03:22, 15 October 2018 (UTC)[reply]
@MusikAnimal: what authentication options do you have configured for this bot account? (e.g. 2FA, BotPasswords, OAuth) — xaosflux Talk 11:57, 15 October 2018 (UTC)[reply]
@Xaosflux: 2FA is enabled. Historically I have not had a good solution for OAuth, but times have changed. I'll try to look into this today. For the record the current consumer for MusikBot II can only edit protected pages, all other admin rights are not permitted. We will use a different consumer here, and all related edits will be tagged with the application name. We could use "GeonoticeSync 1.0" (what I've dubbed the task, and then a version number), or is there a better name? For permissions, I believe the consumer only needs editsiteconfig.
So no need to grant int-admin just yet -- although it should be safe to do so, because we have 2FA enabled and the current consumer can't edit sitewide or user JS/CSS.
The outstanding to-dos:
  1. Create OAuth consumer and rework the bot to use it.
  2. Create Wikipedia:Geonotice/list.json to reflect current configuration, fully protect it, and move Template:Editnotices/Page/User:MusikBot II/GeonoticeSync/Test config.json to Template:Editnotices/Page/Wikipedia:Geonotice/list.json.
  3. Update documentation at Wikipedia:Geonotice and also describe the new process in the comment block at MediaWiki:Gadget-geonotice-list.js.
  4. Ping all the current Geonotice admins to make sure they know about the new system, and the new rules (don't escape single quotes, but do for double, etc.).
  5. Grant int-admin to MusikBot II, and enable the task.
I'll ping you when I'm done with steps 1-3, and once given the final okay we'll do 4-5. Sound like a plan? If we have to rollback or abandon the new system, I'll be sure to let everyone know that they can go back to editing MediaWiki:Gadget-geonotice-list.js directly. — MusikAnimal talk 16:58, 15 October 2018 (UTC)[reply]
Sounds fine, let us know when you are ready. — xaosflux Talk 18:37, 15 October 2018 (UTC)[reply]
@Xaosflux and The Earwig: I spoke with Bawolff, a security expert working for the Foundation. It would seem in this case, bot passwords is no less secure than OAuth. OAuth is more about editing on behalf of users, or authorizing users to use some centralized service. This is fantastic news, because I found the library I was going to use is more for webservices (specifically Ruby on Rails or similar), which doesn't apply here. I would have to implement my own client. To illustrate the complexity, have a look at this "simple" example written in PHP.
So if it's alright, I'd like to move forward with the current bot infrastructure. I have gone ahead and set up the JSON config to reflect the current JS config, and filled in the edit notice. I'm going to be out of town this weekend, so I can start the trial early next week if we're ready to move forward (first doing steps 3-5 above). — MusikAnimal talk 03:09, 19 October 2018 (UTC)[reply]
@MusikAnimal: its not "as good" but it is still much better than using standard passwords. Please let us know what BP grants you are including (you don't have to disclose the allowed IP ranges (that you should also use if you can). — xaosflux Talk 03:15, 19 October 2018 (UTC)[reply]
Just an update that I haven't forgotten about this. I would like to resume work soon. Bawolff had another great idea to use the MediaWiki parser API on the "text". This virtually eliminates security concerns, and above all, gets rid of all that HTML. It'll be easier for Geonotice managers to test what the output looks like, and I assume wikitext is more familiar to them than using <a>...</a>, etc. @Xaosflux and The Earwig: Thoughts? — MusikAnimal talk 23:41, 4 November 2018 (UTC)[reply]
Seems OK, the sooner you can get a mock up running outside of the production page the sooner people can start test cases. — xaosflux Talk 16:03, 5 November 2018 (UTC)[reply]
  • @MusikAnimal, Xaosflux, and Earwig: Thanks for developing this bot! I have a few comments:
    • Coordinates are validated by ensuring there are two corners, and each with two values (lat and lng), and that the values are floats and not integers or strings. - I'm not sure why you want to treat integers and floats differently? Ignore this concern if you've already tested the bot with integer coordinates.
    • Thanks for creating a "comments" field in the draft JSON format. There's no need to copy comments in the JSON to the JavaScript page when the bot syncs notices.
    • Geonotices are a low-volume process and the team of maintainers is small, especially after the IAdmin right got unbundled. I think we should roll out the bot first and then update the documentation as we go along.
      Deryck C. 17:39, 7 November 2018 (UTC)[reply]
      • I've imported all current geonotices into Wikipedia:Geonotice/list.json (involving a bit of search and replace, and a bit of human legwork) so we can test the bot. If it runs correctly none of the current geonotices should change. Deryck C. 17:52, 7 November 2018 (UTC)[reply]
        Good point about the coordinates. I don't know why I decided to enforce floats. This has been fixed.
        For the record, I've tested that the Geonotice actually shows on testwiki. At this point I'm confident to move forward. Glad to hear you are ready as well!
        @Xaosflux: Shall we make a request at BN for the int-admin flag, or can you handle that? — MusikAnimal talk 22:23, 7 November 2018 (UTC)[reply]

Ready to deploy

[edit]

Auto-removal

[edit]

This got me thinking -- the bot could automatically remove expired notices. @Deryck Chan: What do you think? — MusikAnimal talk 18:04, 9 November 2018 (UTC)[reply]

MusikAnimal, Well let's keep some things in place for us int-admins to do. :p —CYBERPOWER (Chat) 18:34, 9 November 2018 (UTC)[reply]
@Redrose64: I'd like to solicit your input as well. I think Cyberpower678 was mostly joking above (maybe? :), but do you think it would make sense for the bot auto-remove expired notices, say, 24 hours afterwards? — MusikAnimal talk 05:59, 19 November 2018 (UTC)[reply]
Auto-removing 1 week after expiry sounds very sensible to me.
Actually I would like to propose a related functionality: perhaps the bot should skip notices whose start date is more than 1 week in the future? That way we can queue notices without burdening other editors' browsers. Deryck C. 11:35, 19 November 2018 (UTC)[reply]
This can be done as well. I want to hear a few other opinions first, though. — MusikAnimal talk 17:39, 19 November 2018 (UTC)[reply]
Seems reasonable to me. If there's something wrong with it, the same editors might not be around/active to notice/fix things, but that's not a change from the original system so there's nothing lost there. It'd make it harder to check if the bot made any errors, since the two pages won't be in sync all the time, but that's not the end of the world nor too hard to manage. ~ Amory (u • t • c) 12:03, 21 November 2018 (UTC)[reply]

Update

[edit]

@The Earwig and MusikAnimal: the int-admin access and the trial are expiring - is this ready to be "trial complete" and reviewed for closure, will you need more time, etc? — xaosflux Talk 03:54, 21 November 2018 (UTC)[reply]

Yes indeed, thanks for the ping. Trial complete. All the relevant edits, and links to the corresponding human edits (in the edit summaries), are at https://en.wikipedia.org/w/index.php?title=MediaWiki:Gadget-geonotice-list.js&action=history&offset=20181121&limit=12. As far as I can tell, it all went smoothly. We didn't get a real-world test of the validation functionality, but that was tested out in the userspace.
I tried the validation functionality for the sake of it. It correctly refused to sync, when one of the JSON entries had non-numerical values in "corners". Deryck C. 11:13, 22 November 2018 (UTC)[reply]
There's also the #Auto-removal feature. Not sure if you want to extend a trial for that, assuming everyone is OK with the idea. — MusikAnimal talk 04:05, 21 November 2018 (UTC)[reply]
Everything seems to have gone swimmingly AFAICS. ~ Amory (u • t • c) 12:06, 21 November 2018 (UTC)[reply]
Agree that the trial has been successful. I think there is value in keeping this discussion page open, so we can continue to discuss the possibility of adding auto-remove functionality, which I think is a "small change to improve the operation of a particular task" (the task brief - to sync valid notices from list.json to geonotice-list.js - is unchanged). Deryck C. 16:47, 21 November 2018 (UTC)[reply]
{{BAGAssistanceNeeded}} any one want to approve or close this? — xaosflux Talk 13:02, 21 November 2018 (UTC)[reply]
@MusikAnimal: OK - so right now your bot's account is sort of broken. The access has expired, but it hasn't come off, and your user groups are stuck and can't be updated. I was going to give this another few days pending a BAG reviewer here, but technically it's borked up. Expect this to break at anytime, if you think its working again (e.g. try adding 'confirmed' to your bot) let me know with a ping please. I haven't opened a phab case on this yet. — xaosflux Talk 03:19, 22 November 2018 (UTC)[reply]
This is... awkward. There are a few recurrent events that I think would want geonotices in the next week or so. But there aren't any open geonotice update requests right now so we can wait a day or two for a BAG member to close this officially.
There's also a wider procedural point to be made: why do we have a BAG procedure where the bot's user-rights expire only 24 hours after the scheduled end time of trial, but bureaucrats aren't supposed to extend the permissions' expiry date while we await formal BAG decision? Deryck C. 12:08, 22 November 2018 (UTC)[reply]
@Xaosflux: Can you confirm whether the problem is: (1) you don't feel comfortable with unilaterally extending MusikBot II's IAdmin flag, or (2) Special:Userrights is buggered so bureaucrats can't extend the IAdmin flag even if someone from BAG approved this now? Deryck C. 12:11, 22 November 2018 (UTC)[reply]
@Deryck Chan: the "technical problem" is that Special:Userrights isn't (or at least wasn't) working for any changes on this one particular account only. — xaosflux Talk 12:38, 22 November 2018 (UTC)[reply]
@MusikAnimal: and @Deryck Chan: whatever was stuck on the account is fixed now, I've extended i-admin for 4 days pending closure. Regarding the normal "trial process" - generally with BRFA's when a trial is conducted it is expected to end, then allow for review of the activities during the trial (at which time the task is not undertaken). We also usually allow for a wide range of discretion with BAG if they want it to work other ways. — xaosflux Talk 12:43, 22 November 2018 (UTC)[reply]
I restarted the bot and it reports that it is running again: Special:PermaLink/870108094. — xaosflux Talk 12:52, 22 November 2018 (UTC)[reply]
Great, thanks for the update. Deryck C. 14:20, 22 November 2018 (UTC)[reply]
 Approved. Thanks to everyone for their work in getting this bot running smoothly. If you'd like to continue the discussion regarding additional minor features within the scope of the task, please feel free to do so below. — Earwig talk 04:24, 23 November 2018 (UTC)[reply]
The above discussion is preserved as an archive of the debate. Please do not modify it. To request review of this BRFA, please start a new section at WT:BRFA.
Retrieved from "https://en.wikipedia.org/w/index.php?title=Wikipedia:Bots/Requests_for_approval/MusikBot_II_2&oldid=870201558"